Skip to content

perf(imputation): cache untreated-projection factorization per fit (#141)#562

Merged
igerber merged 1 commit into
mainfrom
perf/imputation-vproj-factorization-cache
Jun 27, 2026
Merged

perf(imputation): cache untreated-projection factorization per fit (#141)#562
igerber merged 1 commit into
mainfrom
perf/imputation-vproj-factorization-cache

Conversation

@igerber

@igerber igerber commented Jun 27, 2026

Copy link
Copy Markdown
Owner

Summary

  • ImputationDiD conservative-variance: cache the untreated-projection factorization per fit(). The exact imputation projection v = -A_0 (A_0'[W]A_0)^{-1} A_1'w (BJS 2024 Theorem 3) has a target-invariant design (A_0/A_1/factorization) and a target-specific RHS (A_1'w), but previously rebuilt the sparse design and re-spsolved it for every estimand target (overall ATT, each event-study horizon, each group) and again for the bootstrap precompute.
  • Split _compute_v_untreated_with_covariates into _build_untreated_projection (cached) + _solve_untreated_v (per-target): factorize (A_0'[W]A_0) once via scipy.sparse.linalg.factorized and solve only the per-target RHS (factorize-once / solve-many), via a fit-local cache shared across the analytical and bootstrap paths. Collapses O(1 + #horizons + #groups) factorizations (2× with bootstrap) to a single build per fit. Mirrors the TwoStageDiD GMM-sandwich factorized + RuntimeError→dense-lstsq pattern.
  • Closes the Add Borusyak-Jaravel-Spiess (2024) Imputation DiD estimator #141 factorization-cache backlog row.

Methodology references

  • Method: ImputationDiD — Borusyak, Jaravel & Spiess (2024) Theorem 3 conservative variance (the untreated imputation projection / Supplementary Proposition A3).
  • Source: Borusyak, Jaravel & Spiess (2024), Revisiting Event-Study Designs: Robust and Efficient Estimation, ReStud 91(6); see docs/methodology/REGISTRY.md ## ImputationDiD.
  • Intentional deviations: None. Bit-identical to the prior per-target spsolve for a single dense RHS (both use the SuperLU simple driver with the same defaults). No methodology/numerical/public-API change; the REGISTRY "Sparse variance solver" note is updated to describe the factorize-once/solve-many implementation.

Validation

  • Tests added/updated: tests/test_methodology_imputation.py — new TestImputationVarianceFactorizationCache (bit-identity of the cached factorization vs spsolve incl. the survey-weighted branch; cache-reuse is a numerical no-op; projection built exactly once per fit for the analytical, survey, and bootstrap paths; fit-idempotency / no cross-fit leak) and refreshed the singular-Ω₀ fallback test for the new build-time RuntimeError mechanism. tests/test_imputation.py — the two sparse-fallback monkeypatch tests now patch sparse_factorized.
  • Proven bit-identical at atol=0 across FE-only, covariate, survey-weighted, and bootstrap paths (overall + event-study + group SEs). black / ruff / mypy clean (0 new errors).
  • Backtest / simulation / notebook evidence: N/A (internal perf refactor, no behavior change).

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

🤖 Generated with Claude Code

ImputationDiD's conservative-variance untreated projection
v = -A_0 (A_0'[W]A_0)^{-1} A_1'w has a target-invariant design (A_0/A_1
and the factorization of A_0'[W]A_0) and a target-specific RHS (A_1'w),
but previously rebuilt the sparse design and re-spsolve'd it for every
estimand target (overall ATT, each event-study horizon, each group) and
again for the bootstrap precompute.

Split the build (cached) from the solve: factorize (A_0'[W]A_0) once per
fit() via scipy.sparse.linalg.factorized and solve only the per-target
RHS (factorize-once / solve-many), via a fit-local cache shared across
the analytical and bootstrap paths. Collapses O(1+H+G) factorizations
(2x with bootstrap) to a single build per fit. Mirrors the TwoStageDiD
GMM-sandwich factorized + RuntimeError->dense-lstsq pattern.

Bit-identical to the prior per-target spsolve for a single dense RHS
(both use the SuperLU simple driver with the same defaults); proven at
atol=0 across FE-only, covariate, survey-weighted, and bootstrap paths.
No methodology, numerical, or public-API change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

Overall Assessment

Looks good — no unmitigated P0/P1 findings.

Executive Summary

  • Affected method: ImputationDiD conservative variance, BJS 2024 Theorem 3 untreated projection.
  • The factorize-once / solve-many refactor preserves the documented projection form in docs/methodology/REGISTRY.md:L1341-L1345.
  • The solver implementation change is documented in the registry as an implementation note, so it is not a methodology defect: docs/methodology/REGISTRY.md:L1393.
  • proj_cache is threaded through overall, event-study, group, and bootstrap precompute paths.
  • No new inline inference anti-pattern or NaN propagation issue appears in the changed hunks.
  • I could not run tests here because pytest and numpy are not installed in the environment.

Methodology

No findings.

Severity: P3 informational
Impact: The PR changes ImputationDiD variance computation mechanics, but not the estimand or variance formula. The code still computes v_untreated = -A_0 (A_0'[W]A_0)^{-1} A_1'w and only caches target-invariant pieces.
Concrete fix: None required. Relevant code: diff_diff/imputation.py:L1370-L1413, diff_diff/imputation.py:L1529-L1687.

Code Quality

No findings.

Severity: P3 informational
Impact: The cache is fit-local and avoids cross-fit state leakage. The keying choice is justified by same-fit fixed masks/covariates/survey weights.
Concrete fix: None required.

Performance

No findings.

Severity: P3 informational
Impact: The PR removes repeated sparse design construction and factorization across targets. The remaining dense fallback OOM risk is already tracked in TODO.md:L58, so it is not a blocker.
Concrete fix: None required for this PR.

Maintainability

No findings.

Severity: P3 informational
Impact: Splitting build and solve responsibilities makes the variance path easier to reason about and aligns with the existing TwoStageDiD sparse-factorization pattern.
Concrete fix: None required.

Tech Debt

No findings.

Severity: P3 informational
Impact: The completed #141 cache item was removed from TODO, while the separate dense fallback scalability issue remains tracked.
Concrete fix: None required.

Security

No findings.

Severity: P3 informational
Impact: No secrets, I/O, network access, or unsafe deserialization changes in the reviewed diff.
Concrete fix: None required.

Documentation/Tests

No findings.

Severity: P3 informational
Impact: Registry, changelog, fallback tests, bit-identity tests, cache-reuse tests, survey branch coverage, bootstrap propagation, and no-cross-fit-leak coverage were added/updated.
Concrete fix: None required. Targeted tests could not be executed in this environment because pytest/numpy are unavailable.

@igerber igerber added the ready-for-ci Triggers CI test workflows label Jun 27, 2026
@igerber igerber merged commit 4931cdb into main Jun 27, 2026
33 of 34 checks passed
@igerber igerber deleted the perf/imputation-vproj-factorization-cache branch June 27, 2026 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-ci Triggers CI test workflows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant